Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: Add a pre-allocation test #284

Conversation

EchterAgo
Copy link

@EchterAgo EchterAgo commented Oct 11, 2023

I used this test when diagnosing #281 and think it would be a good addition.

This tests file sizes when pre-allocating a file.

Expected values have been confirmed on NTFS, ReFS and FAT32. This test currently does not work on a SMB share with the Samba server because it reports a fixed cluster size based on a configuration option instead of getting the correct value from the underlying file system. ksmbd does not have the same issue, it correctly gets the values from the file system. So far this is untested with Windows SMB shares.

This also makes tests.py use some helpers from utils.py.

Note that currently the test does not pass yet because #281 needs more fixing.

@EchterAgo
Copy link
Author

EchterAgo commented Oct 11, 2023

Note that you can also point this test to a build directory (out/build/x64-Debug for example) using --zfspath and it will use the commands from there without requiring an installation.

@EchterAgo EchterAgo force-pushed the preallocation_test branch 2 times, most recently from aa6aaef to e3b329c Compare October 11, 2023 06:41
@EchterAgo
Copy link
Author

Looks like checkstyle fails because we do not have openzfs@9192ab7

@EchterAgo
Copy link
Author

See openzfs#15284

@EchterAgo
Copy link
Author

Rebased to have the checkstyle fix.

@andrewc12
Copy link

Ahh dang. This actually looks like it might have useful things.
I can't just have terrible python code anymore huh.
I'll look over it sometime.

@EchterAgo
Copy link
Author

@andrewc12 yea, we should work on building a bit of a test library with common functionality. I wanted to also go over tests.py and improve it a bit.

Is there any reason the tests spend so much time sleeping? I'm currently trying to reduce that too.

@andrewc12
Copy link

andrewc12 commented Oct 11, 2023

Mostly extra cautious to avoid BSODs
Thanks for taking a look at this. 😊

@EchterAgo
Copy link
Author

I need to fix the Python style of this change.

@EchterAgo
Copy link
Author

So it looks like currently we only need to wait a bit before destroy, otherwise I get the same BSOD as in #282

@EchterAgo EchterAgo force-pushed the preallocation_test branch 4 times, most recently from b40bb5a to b8e7f1c Compare October 11, 2023 12:28
I used this test when diagnosing openzfs#281 and think it would be a good
addition.

This tests file sizes when pre-allocating a file.

Expected values have been confirmed on NTFS, ReFS and FAT32.
This test currently does not work on a SMB share with the Samba server
because it reports a fixed cluster size based on a configuration option
instead of getting the correct value from the underlying file system.
ksmbd does not have the same issue, it correctly gets the values from
the file system. So far this is untested with Windows SMB shares.

This also makes `tests.py` use some helpers from `utils.py`.

Signed-off-by: Axel Gembe <[email protected]>
@EchterAgo
Copy link
Author

So this should be ready, the failing tests are Ubuntu 22.04 which seems unrelated and test9_regression_test which is the newly added test.

@andrewc12 Did the tests.py test regex for key file ever work? I could never get the original import call to work, but I do have it working now. I still need to make the run out of drive letters test work with the zpool_create context manager. I'll put those updates into a separate PR when this is merged, see https://github.com/EchterAgo/zfs/blob/a4d1fd14300f2991bde059084a885a079be9ad6c/contrib/windows/tests/tests.py

@andrewc12
Copy link

@andrewc12 Did the tests.py test regex for key file ever work? I could never get the original import call to work, but I do have it working now.

I don't know if it ever did, I honestly cant remember

@andrewc12 andrewc12 merged commit 11cb959 into openzfsonwindows:zfs-Windows-2.2.0-release Oct 11, 2023
24 of 30 checks passed
@EchterAgo EchterAgo deleted the preallocation_test branch October 11, 2023 22:53
@andrewc12
Copy link

Yeah this is all good stuff.
I'll have to change the rest of the tests now to use this.

@EchterAgo
Copy link
Author

EchterAgo commented Oct 11, 2023

I was also planning to update the rest of the tests, but let me know if you need any help/additions or have any suggestions for things in utils.py

I want to try to create tests for previous issues I've reported here and have since been fixed.

lundman pushed a commit that referenced this pull request Oct 18, 2023
I used this test when diagnosing #281 and think it would be a good
addition.

This tests file sizes when pre-allocating a file.

Expected values have been confirmed on NTFS, ReFS and FAT32.
This test currently does not work on a SMB share with the Samba server
because it reports a fixed cluster size based on a configuration option
instead of getting the correct value from the underlying file system.
ksmbd does not have the same issue, it correctly gets the values from
the file system. So far this is untested with Windows SMB shares.

This also makes `tests.py` use some helpers from `utils.py`.

Signed-off-by: Axel Gembe <[email protected]>
lundman pushed a commit that referenced this pull request Dec 11, 2023
I used this test when diagnosing #281 and think it would be a good
addition.

This tests file sizes when pre-allocating a file.

Expected values have been confirmed on NTFS, ReFS and FAT32.
This test currently does not work on a SMB share with the Samba server
because it reports a fixed cluster size based on a configuration option
instead of getting the correct value from the underlying file system.
ksmbd does not have the same issue, it correctly gets the values from
the file system. So far this is untested with Windows SMB shares.

This also makes `tests.py` use some helpers from `utils.py`.

Signed-off-by: Axel Gembe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants